-
Notifications
You must be signed in to change notification settings - Fork 8
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[BE] 인수 테스트 에서 사용하는 레포지토리 제거 및 설정 명확하게 변경 (#512) #522
base: develop
Are you sure you want to change the base?
Conversation
Test Results 44 files 44 suites 7s ⏱️ Results for commit 096a2ba. ♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
조하~
말씀하신 대로 컨벤션 오류가 조금 있어서 체크 코멘트만 달았어요.
전반적으로 조금 어렵긴 한데 ㅇ.ㅠ 이전 테스트보다는 훨씬 좋은 것 같네요!
이번에 테스트 쫌쫌따리 추가하면서도 느낀 건데 클래스들이 다 너무 중구난방이더라고요 ㅎㅎ...
조금 어렵더라도 통일해서 쓸 수 있다면 좋을 것 같습니다!
P3 : 컨트롤러 테스트 변경 방향성
그래서 저는 이거 좋은듯요?
한글로 작성된 코드도 아까 구두로 들은 이유 때문이라면 이렇게 유지해도 괜찮을 것 같습니다
P4 효율적 Mocking
저는 전역 Mocking
쪽에 한 표입니다
Gtihub Provider 일관성
저는 1개가 전부 담당하는 편이 좋은 것 같아요~
(크게 보았을 때 하나의 클래스가 담당해도 좋을 것 같음: 같은 성격의 서비스)
import java.util.Arrays; | ||
|
||
public record PullRequestReviews(GithubPullRequestReview[] pullRequestReviews) { | ||
public String getReviewUrl(String githubUserId) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public String getReviewUrl(String githubUserId) { | |
public String getReviewUrl(String githubUserId) { |
import java.util.List; | ||
|
||
public class MatchingGiven { | ||
public static void 매칭(String accessToken, long roomId) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public static void 매칭(String accessToken, long roomId) { | |
public static void 매칭(String accessToken, long roomId) { |
import io.restassured.http.ContentType; | ||
|
||
public class ReviewGiven { | ||
public static void 리뷰_완료(String accessToken, long roomId, long revieweeId) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public static void 리뷰_완료(String accessToken, long roomId, long revieweeId) { | |
public static void 리뷰_완료(String accessToken, long roomId, long revieweeId) { |
import java.util.List; | ||
|
||
public class RoomGiven { | ||
public static RoomResponse 방생성(String accessToken, int matchingSize) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public static RoomResponse 방생성(String accessToken, int matchingSize) { | |
public static RoomResponse 방생성(String accessToken, int matchingSize) { |
.then().assertThat().statusCode(201).extract().as(RoomResponse.class); | ||
//@formatter:on | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.map(MatchResultResponse::userId) | ||
.orElseThrow(); | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assertThatThrownBy(() -> pullRequestReviews.getReviewUrl("notExist")) | ||
.isInstanceOf(CoreaException.class); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
}; | ||
|
||
@Test | ||
@DisplayName("리뷰가 존재하면, 리뷰의 URL 을 가져온다.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DisplayName("리뷰가 존재하면, 리뷰의 URL 을 가져온다.") | |
@DisplayName("리뷰가 존재하면, 리뷰의 URL을 가져온다.") |
} | ||
|
||
@Test | ||
@DisplayName("리뷰가 존재하지 않는 ID 를 넣으면 예외를 발생한다.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DisplayName("리뷰가 존재하지 않는 ID 를 넣으면 예외를 발생한다.") | |
@DisplayName("리뷰가 존재하지 않는 ID를 넣으면 예외를 발생한다.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
일단 토론의 초석 느낌이라 하셔서 approve는 누르고 시작할게요~
P3 : 컨트롤러 테스트 변경 방향성
말해준 방향성 너무 좋습니다.
한글로 작성하는 것도 상관없어요~
Service , Repository 의존성 최대한 제거 ( 보시면, Import 문 Mocking 부분 뺴고 다 제거 되었습니당 )
실제 요청을 통한 검증
정확한 인수 테스트라 할 수 있겠네요! 👍👍👍
P4 효율적 Mocking
어차피 여러 인수 테스트에서 중복으로 사용된다면, 전역 Mocking 좋은 것 같아요.
방법은 제가 생각할 때 AcceptanceTest
(이름은 대충 썼음)라는 추상 클래스를 만들고 이 부분에서 Mocking 해준 후 다른 테스트들이 상속받는 구조가 제일 바람직하다 생각합니다.
P4 Gtihub Provider 일관성
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
테스트 통일 작업이 쉽지 않았을텐데, 좋은 방향 제시해주서 고마워요 ㅎㅎ
패키지 이름은 Behavior Driven Development 에서 온거겠죠??
컨트롤러 테스트 변경 방향성
수많은 레포지토리와 서비스로부터 의존을 끊어내고 실제 요청으로 컨트롤러 테스트가 돌아가니 좋네요. 정말 제대로 api 를 테스트하는 느낌이어서 좋습니다!
효율적 Mocking
저도 전역 mocking 이 괜찮아 보이네요!
Gtihub Provider 일관성
이 부분은 하나로 합쳐도 괜찮을 것 같아요. Github API 를 중계하는 역할이 나뉠 필요는 없을 것 같다는 생각이 듭니다!
📌 관련 이슈
✨ PR 세부 내용
해당 내용은 바로 머지하려고 하긴 보다 더 좋은 코드를 위한 토론의 초석 느낌입니다.
P3 : 컨트롤러 테스트 변경 방향성
이와 같이 인수 테스트를 작성해봤습니다. ( 한글은 각자 기호 차이 )
신경쓴 부분은
이렇게 테스트를 작성하며 느낀점으론
해당 로직이 무조건 Request - Response 단위에서 동작함을 보장할 수 있습니다.
(
When
을 위한Given
도 실제 요청 이므로, 사용자가 행하는 로직과 동일(유사)하다 )추가로, 우리가 코드를 잘 짜고 있는지? 에 대해서도 생각을 하게 도와주는 거 같습니다.
(
MatchResultRespone
가 각 MatchResult 의 ID 를 가지고 있고, 이를 통해 조회를 해도 되지 않을까? 라는 생각이 들었어용-> 인덱스 용이 )
하지만, 좀 복잡하거나 힘들다고 하면 힘든것도 사실 인듯용 🥲
이게 괜찮다면, 다른 컨트롤러 테스트도 이와 같이 변경해나갈 거 같습니다.
P4 효율적 Mocking
이와같이 setup 부분에서
Mocking
을 지정하는 부분이 있습니다.이 부분을 사용하려면 모든 부분에서 목 설정을 해야만 합니다.
이를 좀 더 효율적으로 변경할 필요가 있는거 같습니다.
PR 올렸는지 & 리뷰 했는지 확인하는 부분을 인터페이스로 분리 or 전역 Mocking 할 방법을 찾는걸 생각했는데
다양한 의견들을 주세용 🙂
P4 Gtihub Provider 일관성
p.s 현재,
GtihubOAuthProvider
가 Review 까지 조회하는걸 확인했는데현재 저희가 리뷰 조회 & PR 조회 & 유저 조회 3가지의 Github API 로직을 사용하는데
이들을 하나의 Provider 가 담당하게 할지, 각각 담당하게 할지 얘기해봐야 할 거 같아용.
( 현재는 1개 / 2개 가지고 있음 )
이상이고, 초안 느낌으로 올린거라 컨벤션 어디 틀린 부분들도 있을듯용☺️